Skip to content
This repository was archived by the owner on May 15, 2025. It is now read-only.

[DO NOT MERGE] Renamed Scheduler APIs to better reflect their usage. #116

Closed
wants to merge 44 commits into from

Conversation

shmuelk
Copy link
Collaborator

@shmuelk shmuelk commented May 4, 2025

The PR renames the two APIs of the Scheduler. In particular:

  • Scheduler.Schedule has been renamed scheduler.OnRequest to better indicate that it is invoked during request processing.
  • Scheduler.RunPostResponsePlugins has been renamed to Scheduler.OnResponse to both indicate that it is invoked during response processing and to better decouple it from PostResponse plugin handling

In addition the code in the function Scheduler.OnResponse that is specific to PostResponse plugin handling has been moved to a new private function Scheduler.runPostResponsePlugins, in the style of th functions that drive the handling of the other plugins.

@shmuelk shmuelk requested a review from elevran May 4, 2025 14:33
Copy link
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to not change Schedule, but rather add another interface type (ie and not a function on an existing interface) for doing the response processing.

@elevran
Copy link
Collaborator

elevran commented May 6, 2025

/hold
Should minimize changes to scheduler and upstream

@elevran elevran changed the title Renamed Scheduler APIs to better reflect their usage. [DO NOT MERGE] Renamed Scheduler APIs to better reflect their usage. May 6, 2025
@clubanderson clubanderson force-pushed the dev branch 2 times, most recently from 9ef49b8 to 5ee8225 Compare May 7, 2025 14:44
@elevran
Copy link
Collaborator

elevran commented May 8, 2025

@shmuelk - please close as this is being worked on in other PRs

@shmuelk shmuelk closed this May 8, 2025
@shmuelk shmuelk deleted the post-response-2 branch May 15, 2025 13:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants